Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[26737] Made sure that the sections from the request money are scroll… #26781

Closed
wants to merge 2 commits into from
Closed

[26737] Made sure that the sections from the request money are scroll… #26781

wants to merge 2 commits into from

Conversation

DanutGavrus
Copy link
Contributor

As this is a draft PR, I'm adding here a video with before(latest staging as of now) and after(this PR changes) so we may decide if we want(or not) to proceed with these changes:

scroll_1.mp4
scroll_2.mp4
scroll_3.mp4

@DanutGavrus
Copy link
Contributor Author

Differences in MoneyRequestSelectorPage are strangely shown, I've just added a <ScrollView contentContainerStyle={[styles.flexGrow1]}> around the OnyxTabNavigator and nothing else.

@DanutGavrus
Copy link
Contributor Author

DanutGavrus commented Sep 5, 2023

The comparison was made against the latest staging, when on main, we have this PR: #26728 which added the <ScrollView contentContainerStyle={styles.flex1}> in the distance request. The only difference on main is that we may now scroll down to the button, but we lose the bottom margin when we scroll as seen here(I can't scroll anymore):
image

And we still have 1 problem left, we have functionality in code which wants to show 4 locations before making the list scrollable, and to also show only half of the fifth and up location as to indicate that the list is scrollable as seen here:

const MAX_WAYPOINTS_TO_DISPLAY = 4;

// Show up to the max number of waypoints plus 1/2 of one to hint at scrolling
const halfMenuItemHeight = Math.floor(variables.baseMenuItemHeight / 2);
const scrollContainerMaxHeight = variables.baseMenuItemHeight * MAX_WAYPOINTS_TO_DISPLAY + halfMenuItemHeight;

But doesn't work on current main.

@DanutGavrus
Copy link
Contributor Author

I'll close this based on this discussion: https://expensify.slack.com/archives/C049HHMV9SM/p1693986149607419. We've fixed the mentioned issues on the main branch by replacing flex1 with flexGrow1 in the distance page. This PR's only remaining benefit would be that it would remove all scroll views from sections and place only 1 upper in the hierarchy, making any existing & future section scrollable. We don't know for sure that it won't introduce new problem and It doesn't seem required for now.

@DanutGavrus DanutGavrus closed this Sep 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant